fix: skip codex config.toml rewrite when there's nothing to merge by chelsealong · Pull Request #4564 · github/spec-kit

chelsealong · 2026-09-13T02:34:50Z

Problem

When Codex has no Specify-managed event hooks configured (the exact
scenario in #4563: a fresh specify integration install codex with no
extensions/overrides declaring any handlers), resolve_events() returns
{}, and install_integration_events() routes through its
empty-resolved-map branch into _remove_native_event_hooks() ->
_remove_toml_entries() — not through _merge_toml_fragment().

_remove_toml_entries() always rewrote the destination file, even when
stripping Specify-owned hook blocks was a no-op (no such blocks were
present) — i.e. a true no-op install/teardown against a config.toml with
no Specify content at all.

That unconditional rewrite went through Python's text-mode read_text()/
write_text(), which perform newline translation. On Windows this
silently turned an LF-terminated pre-existing .codex/config.toml into
CRLF, producing a git-visible diff with no semantic content change.
Because the file is outside the Codex/Spec Kit managed manifests in this
scenario, specify integration status --json reports a clean/healthy
state while git diff shows the file as modified — exactly as described
in #4563.

An earlier version of this PR patched _merge_toml_fragment() instead.
That function does have the identical unconditional-rewrite shape, but it
is only reached when there is at least one supported, non-empty event to
merge — the empty-map/no-op case never calls it, so that fix had no effect
on the actual bug. This revision fixes _remove_toml_entries(), the
function that is actually executed on the issue's repro path (confirmed
by tracing every caller of install_integration_events/resolve_events
and by instrumented runs of the CLI's specify init / specify integration install codex against a real .codex/config.toml).

Fix

_remove_toml_entries() now skips the write (return False, leaving the
file completely untouched — byte-for-byte, including its original line
endings) when stripping Specify-owned blocks left the content unchanged,
mirroring the existing "unreadable file" skip path already in this
function and the analogous guard already present in
_merge_toml_fragment().

The previous, harmless-but-insufficient _merge_toml_fragment() guard is
left in place (it doesn't hurt, and does fix a separate reachable no-op
case where a fragment is being merged but the file already contains it
identically).

Test

Replaced TestTomlNoOpMerge with
test_no_events_leaves_existing_config_untouched, which drives the real
production shape: install_integration_events(integration, tmp_path, manifest, {}) — the empty resolved-events map resolve_events() actually
returns when no hooks are configured — against a pre-existing
.codex/config.toml with no Specify-owned content. It asserts both that
the file's bytes are unchanged and that the file's mtime is unchanged
(byte-equality alone doesn't catch an unconditional rewrite on Linux,
where the platform line separator is already \n; the mtime check
verifies no write occurs at all, which is what actually mangles line
endings on Windows).

Confirmed the test fails without this revision's fix (git checkout HEAD -- src/specify_cli/events.py to restore the pre-this-fix version, which
still has the previous PR's _merge_toml_fragment-only patch):

FAILED tests/integrations/test_events.py::TestTomlNoOpMerge::test_no_events_leaves_existing_config_untouched
AssertionError: assert 1789266231860510616 == 1789266231859813750

With the fix applied:

tests/integrations/test_events.py::TestTomlNoOpMerge::test_no_events_leaves_existing_config_untouched PASSED
129 passed in 1.09s

Full suite (python3 -m pytest tests -q): 8054 passed, 12 skipped.

ruff check on the changed files shows the same pre-existing findings as
before this change; none are on the added/modified lines.

AI assistance disclosure

This PR was authored by an autonomous AI coding agent (Claude Code). An
independent review (also AI-assisted) found that the original version of
this PR patched the wrong function; this revision traces the issue's
actual repro path, fixes the function that is really executed
(_remove_toml_entries), and rewrites the regression test to exercise
that real path instead of a synthetic input no production caller
produces.

_merge_toml_fragment() always rewrote .codex/config.toml, even when the event fragment was empty and there were no Specify-owned hook blocks to remove. That unconditional rewrite appended stray blank lines and, via Python's text-mode newline translation on read/write, silently changed the file's line-ending convention (LF -> CRLF on Windows) — turning a no-op install into a spurious, unmanifested diff on a pre-existing tracked file. Now the merge is skipped (and the file left untouched) when there is no fragment to add and no owned blocks to remove, matching the existing S5 tracking convention used by the other native-format mergers in this file.

…p path Review found the prior fix patched the wrong function: the real "specify integration install codex" repro (no Codex event hooks configured) resolves to events={}, which routes through install_integration_events's empty-map branch into _remove_native_event_hooks -> _remove_toml_entries, never touching _merge_toml_fragment. _remove_toml_entries still rewrote the file unconditionally even when the regex strip was a no-op, which (via text-mode newline translation) mangles line endings on Windows. Adds the same cleaned == existing guard to _remove_toml_entries, and replaces the regression test with one that drives the real install_integration_events(..., events={}) path instead of a synthetic events map no caller can produce.

mnriem · 2026-09-16T12:18:52Z

The revised fix reaches the correct cleanup path, but the no-op guard runs too late. A comments-only .codex/config.toml with no Specify-owned blocks is still deleted before cleaned == existing is checked.

Please check for unchanged content before the empty/comments-only deletion branch. Cover preservation of comments-only and empty unowned files, while retaining cleanup when Specify-owned blocks were actually removed.

Also correct the description’s claim about identical nonempty fragments—the merge guard only handles an empty fragment—and add the model(s) used to the existing AI disclosure.

Drafted for @mnriem with assistance from GitHub Copilot (model: GPT-6 Astra; interactive comment drafting).

Copilot

🟡 Changes recommended

Blank or comment-only user configurations can still be deleted before the new no-op guard runs.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Prevents Codex event setup from rewriting unchanged user-owned TOML configuration.

Changes:

Adds no-op guards to TOML merge and cleanup paths.
Adds a byte/mtime regression test for empty event installation.

File summaries

File	Description
`src/specify_cli/events.py`	Skips unnecessary TOML writes.
`tests/integrations/test_events.py`	Tests preservation of existing Codex configuration.

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    if cleaned == existing:
+        return False


chelsealong added 2 commits September 13, 2026 02:15

chelsealong requested a review from mnriem as a code owner September 13, 2026 02:34

This was referenced Sep 15, 2026

Post-v2 simplification: Spec Kit clean adoption と custom tooling の標準化 reitojike/stage-tracker#434

Open

Post-v2 harness canary: GitHub Spec Kit を non-authoritative に導入検証する reitojike/stage-tracker#439

Closed

mnriem requested a balanced review from Copilot September 16, 2026 12:18

Copilot started reviewing on behalf of mnriem September 16, 2026 12:19 View session

mnriem added author-needs-disclosure AI use, or the agent/model/settings behind it, not disclosed per CONTRIBUTING author-awaiting Waiting on author response labels Sep 16, 2026

Copilot AI reviewed Sep 16, 2026

View reviewed changes

Comment thread src/specify_cli/events.py

Comment on lines +2243 to +2244

if cleaned == existing:

return False

Navigation Menu

Uh oh!

fix: skip codex config.toml rewrite when there's nothing to merge - #4564

fix: skip codex config.toml rewrite when there's nothing to merge#4564
chelsealong wants to merge 2 commits into
github:mainfrom
chelsealong:fix/4563-codex-toml-noop-merge

chelsealong commented Sep 13, 2026

Uh oh!

mnriem commented Sep 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

chelsealong commented Sep 13, 2026

Problem

Fix

Test

AI assistance disclosure

Uh oh!

mnriem commented Sep 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants